Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update version range for Gson to '[2.9.1,2.11)' #690

Merged
merged 1 commit into from
Feb 28, 2023

Conversation

HannesWell
Copy link
Contributor

@cdietrich
Copy link
Contributor

cdietrich commented Jan 7, 2023

see also #689
am not sure which variant we want

@HannesWell
Copy link
Contributor Author

see also #689 am not sure which variant we want

Yours definitely looks more complete. And 2.10 also just added new methods (see eclipse-m2e/m2e-core#1146 (comment)). So if lsp4j is using gson as consumer and not as provider, the lower can actually stay where it is. If that is wanted it is probably better to update your PR accordingly. And if not yours looks more complete.

So either way this can be closed I think.
Thanks for the hint.

@HannesWell
Copy link
Contributor Author

@jonahgraham reopened due to eclipse-pde/eclipse.pde#482.

@cdietrich
Copy link
Contributor

@HannesWell is there any documentation on versioning policy on googles side?

@HannesWell
Copy link
Contributor Author

Unfortunately not really, no.
The only thing I found is a Policy for guava:
https://github.com/google/guava/wiki/ReleasePolicy

But this obviously does not apply to gson.

@@ -16,7 +16,7 @@ ext.versions = [
'xtend_lib': '2.28.0',
'guava': '[30.1,31)',
'guava_orbit': '30.1.0.v20221112-0806',
'gson': '[2.10.1,2.11)',
'gson': '[2.9.1,2.11)',
'gson_orbit': '2.10.1.v20230109-0753',
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the value of gson_orbit used? I couldn't find any reference when searching the repo in the GH UI.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

afaik not anymore. just as a reference for the tycho stuff under releng.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i also expect the other places to be adjusted e.g. feature.xml

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

afaik not anymore. just as a reference for the tycho stuff under releng.

OK, removed it. Lets see if the build succeeds.

i also expect the other places to be adjusted e.g. feature.xml

Thanks for the hint. Adjusted the feature too and changed the version rule to compatible to not bind it to gson 2.9.
But I wonder why there is a dedicated dependency for gson in the feature at all?
Since the feature contains org.eclipse.lsp4j already it implicitly requires gson already anyway.
And p2 will consider the version specified in org.eclipse.lsp4j when installing the feature. Therefore this looks like a duplicated information to me. Probably the same for xtend.lib?

In general Feature-dependencies are actually only useful/necessary if none of the included Plug-ins requires it.

@cdietrich
Copy link
Contributor

please note: we at xtext wont be able to build a new release to include this fix for 2023-03

@HannesWell
Copy link
Contributor Author

please note: we at xtext wont be able to build a new release to include this fix for 2023-03

Understand. Does xtext use similar tight version ranges on gson like lsp4j? If not it is hopefully less problematic.

@jonahgraham
Copy link
Contributor

BTW, I expect xtext to be strictly dependent on lsp4j 0.20.0, so a new lsp4j but no new xtext will mean three versions of lsp4j?

@HannesWell
Copy link
Contributor Author

BTW, I expect xtext to be strictly dependent on lsp4j 0.20.0, so a new lsp4j but no new xtext will mean three versions of lsp4j?

If the version range is very strict and does not even allow a 0.20.1, then yes. :/

I tried it, but didn't find out where exactly xtext depends on lsp4j? @cdietrich can you please clarify in this regard?

@jonahgraham
Copy link
Contributor

If the version range is very strict and does not even allow a 0.20.1, then yes. :/

OK. Thank you for pointing this part out as I otherwise would have done a 0.21.0 release. We (LSP4J) haven't done a x.x.1 release in a while, but I can give it a go.

@jonahgraham
Copy link
Contributor

I tried it, but didn't find out where exactly xtext depends on lsp4j? @cdietrich can you please clarify in this regard?

See eclipse/xtext-core#2019 for xtext tracking issue. It has links to all the code updates needed for LSP4J change.

@HannesWell
Copy link
Contributor Author

If the version range is very strict and does not even allow a 0.20.1, then yes. :/

OK. Thank you for pointing this part out as I otherwise would have done a 0.21.0 release. We (LSP4J) haven't done a x.x.1 release in a while, but I can give it a go.

I just checked the content metadata in https://download.eclipse.org/modeling/tmf/xtext/updates/releases/2.30.0/ and it shows me that org.eclipse.xtext.ide requires org.eclipse.lsp4j(.jsonrpc) [0.20.0,0.21.0)

        <required namespace='osgi.bundle' name='org.eclipse.lsp4j' range='[0.20.0,0.21.0)' optional='true' greedy='false'/>
        <required namespace='osgi.bundle' name='org.eclipse.lsp4j.jsonrpc' range='[0.20.0,0.21.0)' optional='true' greedy='false'/>

The Plugin org.eclipse.xtext.testing just requires lsp4j without a version range. Besides that nothing else in xtext seems to require lsp4j/e.

Therefore I think a 0.20.1 lsp4j should fully supersede lsp4j 0.20.0 in SimRel. But maybe @merks can also check with his tools.

@cdietrich
Copy link
Contributor

cdietrich commented Feb 28, 2023

Yes we would accept a 0.20.1 I guess. also the deps are optional afaik
But I don’t know what happens with tycho
When maven and eclipse dependencies are mixed
As our bom (see Xtext-lib) points to 0.20.0 as maven version
And newer tycho versions also seem keen to look at maven coords

@cdietrich
Copy link
Contributor

cdietrich commented Feb 28, 2023

native question: a lsp4e bump based on the last release with just lsp4j/gson dep. bumped is not an option?

@HannesWell
Copy link
Contributor Author

But I don’t know what happens with tycho
When maven and eclipse dependencies are mixed
As our bom (see Xtext-lib) points to 0.20.0 as maven version

I cannot tell that. Maybe @laeubi knows in detail? But at least in the p2-world (or Tycho builds based on targets) maven bom's are not considered AFAIK.

native question: a lsp4e bump based on the last release with just lsp4j/gson dep. bumped is not an option?

I wondered that as well. @mickaelistria would it be?

@merks
Copy link

merks commented Feb 28, 2023

@HannesWell

Yes xtext would accommodate a 0.20.1 version of lsp4j. Is the only thing strictly requiring >= 0.20.0:

image

If you have snapshot versions of your proposed changes available somewhere I can repeat the analysis. I can even locally build the aggregation and use that to test creating the DSL package, so it would be really great to have snapshot versions (of lsp4j and m2e I guess?) ASAP...

@cdietrich
Copy link
Contributor

the tycho maven stuff is this one: eclipse/xtext-eclipse#1931
as our p2 stuff is created from maven artifact, tycho seems to look at maven artifacts.
so i dont know what would happen.

@cdietrich
Copy link
Contributor

@merks you can find p2 repo for this pr at https://ci.eclipse.org/lsp4j/job/lsp4j-github-pullrequests/399/artifact/build/p2-repository/

@HannesWell
Copy link
Contributor Author

@merks you can find p2 repo for this pr at https://ci.eclipse.org/lsp4j/job/lsp4j-github-pullrequests/399/artifact/build/p2-repository/

Thanks. Btw. is there a reason, why you don't have a dedicated job per PR, for example like in m2e?
https://ci.eclipse.org/m2e/job/m2e/
This makes it easier to track the results of one PR.

If you have snapshot versions of your proposed changes available somewhere I can repeat the analysis. I can even locally build the aggregation and use that to test creating the DSL package, so it would be really great to have snapshot versions (of lsp4j and m2e I guess?) ASAP...

That is what I plan to contribute to SimRel for M2E tonight:
https://download.eclipse.org/technology/m2e/snapshots/latest/

@cdietrich
Copy link
Contributor

i see it for my branches, they are at eclipse/lsp4j, but you pr'ed from your account

@HannesWell
Copy link
Contributor Author

i see it for my branches, they are at eclipse/lsp4j, but you pr'ed from your account

Yes, but the way for example the m2e Jenkins project is set up (I don't know exactly what the infra team did different) and AFAICT for all platform TLP projects, a new job is created for each PR in a dedicated tab, regardless from where the branch is coming. See for example PDE:
https://ci.eclipse.org/pde/job/eclipse.pde/view/change-requests/

@merks
Copy link

merks commented Feb 28, 2023

That looks better with only features requiring the higher version of gson.

image

And xtext.ide resolving:

image

I'll try to build the aggregation locally (with my horrible internet here)....

@merks
Copy link

merks commented Feb 28, 2023

Woo hoo!

The DSL package installed from the aggregated result of the new m2e and this lsp4j version contains only a single version of gson:

image

Visiting all preference pages which tends to activate all bundles works well too.

I will now test the JEE package as well but given it was working fine before I don't expect a problem....

@laeubi
Copy link

laeubi commented Feb 28, 2023

with only features requiring the higher version of gson

It might be good to not include such dependencies in features, they should be pulled in by their using bundle anyways?

@merks
Copy link

merks commented Feb 28, 2023

Indeed, though often when the platform has tried to remove includes, something stopped working as @akurtakov can confirm. Also, includes are often present because that helps control what's included in the p2 repository; not everyone can or should just include all dependencies, which is a nice and easy solution when you are at the bottom of the food chain and your dependencies tend to be mostly just third party ones...

@merks
Copy link

merks commented Feb 28, 2023

The JEE package still has two versions:

image

But none of the exports have been substituted:

g! b 10
com.google.gson_2.9.1.v20220915-1632 [10]
Id=10, Status=RESOLVED Data Root=D:\Users\test17\jee-latest-local-aggr\eclipse\configuration\org.eclipse.osgi\10\data
"No registered services."
No services in use.
Exported packages
com.google.gson.internal; version="2.9.1"[exported]
com.google.gson.internal.bind; version="2.9.1"[exported]
com.google.gson.internal.bind.util; version="2.9.1"[exported]
com.google.gson.internal.reflect; version="2.9.1"[exported]
com.google.gson.internal.sql; version="2.9.1"[exported]
com.google.gson; version="2.9.1"[exported]
com.google.gson.annotations; version="2.9.1"[exported]
com.google.gson.reflect; version="2.9.1"[exported]
com.google.gson.stream; version="2.9.1"[exported]
Imported packages
sun.misc; version="0.0.0" <org.eclipse.osgi_3.18.300.v20230211-2021 [0]>
No fragment bundles
No required bundles

So that still looks fine.

@laeubi
Copy link

laeubi commented Feb 28, 2023

I think we addressed already a lot of the "something", so I think its worth to try some things (again) e.g. we have now:

So one can use "include all" but still filter items provided from another update-site, and one can include any source if desired (not sure how good this applies to the epp / simrel builds)

@merks
Copy link

merks commented Feb 28, 2023

@jonahgraham

FYI, it looks like we are zeroing in on a solution to the gson/lsp4j problem!

@akurtakov
Copy link

Platform issues when removing includes are exactly that things didn't end up in the p2 site so as long as one verifies the proper content is in the p2 site I believe using import is better.

@merks
Copy link

merks commented Feb 28, 2023

@akurtakov We also ran into strange problems with removing includes from the test feature and we didn't have time to track that down. I totally agree we should try this again during the 4.28 release cycle. It would make updating the target platform much less painful and way less time consuming...

@merks
Copy link

merks commented Feb 28, 2023

@laeubi

We digress, but this sounds interesting!

https://github.com/eclipse-tycho/tycho/blob/master/RELEASE_NOTES.md#new-parameter-for-tycho-p2-repository-pluginassemble-repository

I found the docs here:

https://tycho.eclipseprojects.io/doc/master/tycho-p2/tycho-p2-repository-plugin/assemble-repository-mojo.html

It's not entirely clear what a "referenced repository" is but I guess that's a repository referenced from the category.xml; that would indeed be a useful feature...

@laeubi
Copy link

laeubi commented Feb 28, 2023

@merks contributions for better wording are always welcome, developers (at least me) are often the wrong person to write docs because for them everything is "clear from the code"

This is how it is used with m2e:
https://github.com/eclipse-m2e/m2e-core/blob/e466baf6a855762072bb487fc4858db1a11ff9c2/org.eclipse.m2e.repository/category.xml#L30-L32

@jonahgraham jonahgraham modified the milestones: 0.20.0, 0.20.1, 0.21.0 Feb 28, 2023
@jonahgraham
Copy link
Contributor

Thanks all for your input and tests. I will ASAP make a 0.20.1 release that contains this change.

@jonahgraham jonahgraham changed the base branch from main to release_0.20 February 28, 2023 15:06
@jonahgraham jonahgraham merged commit ff58da7 into eclipse-lsp4j:release_0.20 Feb 28, 2023
@HannesWell HannesWell deleted the updateGson branch February 28, 2023 15:58
@HannesWell
Copy link
Contributor Author

Thanks all for your input and tests. I will ASAP make a 0.20.1 release that contains this change.

Thank you Jonah for performing this release.
Should I create another PR targeting main, so that we also have the wider Version range for 0.21?

@jonahgraham
Copy link
Contributor

Should I create another PR targeting main, so that we also have the wider Version range for 0.21?

I did that already but I hadn't pushed it yet. It is now here in #701

jonahgraham pushed a commit to jonahgraham/lsp4j that referenced this pull request Apr 11, 2023
jonahgraham pushed a commit that referenced this pull request Apr 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants